[breaking] daemon: Fix concurrency and streamline access to PackageManager #1828

cmaglie · 2022-08-09T09:10:33Z

Please check if the PR fulfills these requirements

The PR has no duplicates (please search among the Pull Requests
before creating one)
The PR follows
our contributing guidelines
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)
UPGRADING.md has been updated with a migration guide (for breaking changes)

What kind of change does this PR introduce?
Makes access to PackageManger thread-safe and transactional allowing multiple thread concurrent access.
This is not a problem when the arduino-cli is run from the command line (since there is no concurrency in this case), but it can be problematic in daemon mode, when a thread may read the package manager status while another one is writing on it (for example a thread may do a BoardSearch while another one is doing a PlatformsInstall or an Init).

What is the current behavior?
The CLI daemon may crash in some specific circumstances.

What is the new behavior?
The CLI daemon should handle gracefully any combination of gRPC requests.

Does this PR introduce a breaking change, and is titled accordingly?
Yes, docs are ~~not yet~~ updated.

Other information:
This PR is built on top of Fix gRPC BoardList* methods concurrency issues #1804 and must be merged together with it.

codecov · 2022-08-09T09:20:09Z

Codecov Report

Merging #1828 (009dc94) into daemon-fixes (9c334ed) will decrease coverage by 0.13%.
The diff coverage is 36.86%.

❗ Current head 009dc94 differs from pull request most recent head df28c7c. Consider uploading reports for the commit df28c7c to get more accurate results

@@               Coverage Diff                @@
##           daemon-fixes    #1828      +/-   ##
================================================
- Coverage         36.47%   36.33%   -0.14%     
================================================
  Files               232      231       -1     
  Lines             19401    19550     +149     
================================================
+ Hits               7076     7103      +27     
- Misses            11500    11617     +117     
- Partials            825      830       +5

Flag	Coverage Δ
unit	`36.33% <36.86%> (-0.14%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
arduino/cores/packagemanager/profiles.go	`0.00% <0.00%> (ø)`
arduino/cores/status.go	`56.07% <ø> (ø)`
cli/arguments/completion.go	`0.00% <0.00%> (ø)`
cli/arguments/port.go	`0.00% <0.00%> (ø)`
cli/board/list.go	`0.00% <0.00%> (ø)`
cli/compile/compile.go	`0.00% <0.00%> (ø)`
cli/lib/upgrade.go	`0.00% <0.00%> (ø)`
cli/upload/upload.go	`0.00% <0.00%> (ø)`
commands/board/attach.go	`0.00% <0.00%> (ø)`
commands/board/details.go	`0.00% <0.00%> (ø)`
... and 55 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Ref: arduino/arduino-cli#1828 Signed-off-by: Akos Kitta <[email protected]>

It has been splitted and merged into HardwareLoader and TargetBoardResolver that are more appropriate.

cmaglie · 2022-08-10T13:13:09Z

This PR now targets arduino:daemon-fixes branch, the docs will be updated later or in another PR.

commands/instances.go

arduino/cores/packagemanager/package_manager.go

commands/instances.go

commands/core/install.go

commands/core/uninstall.go

commands/core/upgrade.go

umbynos · 2022-08-11T10:37:17Z

legacy/builder/hardware_loader.go

+		}
+
+		pm := pmb.Build()
+		pme, _ /* never release... */ := pm.NewExplorer()


this is really ugly, maybe bringing this inside the arduino builder should solve the problem

Unfortunately it's more complicated than that, we have to keep it for now.
Let's open a dedicated issue to keep track of it and fix it in the future.

arduino/cores/packagemanager/package_manager.go

commands/instances.go

They have been moved into a Builder object that has the ability to build a new PackageManager. This allows to clearly separate subrotuines that actually change the status of the PackageManager from subroutines that just need to query it.

The Explorer object can be see as a read-only "view" to the underlying PackageManager: we may ask the PackageManager to create an Explorer on itself. The returned explorer will held a read-lock on the PackageManager until it's disposed. This architecture should prevent unwanted changes on the PackageManager while it's being used, and viceversa, when the PackageManager is updated it should be guaranteed that no Explorers are reading it.

… calling commands.Init Otherwise, since Init will try to take a write-lock, it will block indefinitely.

This container will handle all the atomic access to the instances map.

It has also been deprecated in favor of GetPackageManagerExplorer.

Previuosly the methods PackageManager.NewBuilder and PackageManager.NewExplorer were available also on Builder and Explorer. Now Builder and Explorer does not inherith these methods anymore, avoiding trivial errors like the one fixed in this commit in the builder_utils package.

arduino/cores/packagemanager/package_manager.go

commands/instances.go

rpc/cc/arduino/cli/commands/v1/common.go

docs/UPGRADING.md

Co-authored-by: per1234 <[email protected]>

per1234

I verified that the recent round of changes fixed the known defect in the IDE that occurred when using the previous version: arduino/arduino-ide#1330

I have also done a lot of general testing of the Arduino IDE 2.x integration without discovering any problems.

Thanks for your excellent work both on the code and the documentation Cristian!

cmaglie force-pushed the allow_core_search_and_init branch from e4ed93a to de6ae8c Compare August 9, 2022 09:13

kittaakos mentioned this pull request Aug 9, 2022

Use the refactored CLI in IDE arduino/arduino-ide#1310

Merged

4 tasks

cmaglie linked an issue Aug 9, 2022 that may be closed by this pull request

Investigate possible concurrency issue in gRPC daemon during Init #1801

Closed

3 tasks

cmaglie self-assigned this Aug 9, 2022

cmaglie added priority: high Resolution is a high priority type: imperfection Perceived defect in any part of project topic: CLI Related to the command line interface criticality: high Of high impact labels Aug 9, 2022

kittaakos pushed a commit to arduino/arduino-ide that referenced this pull request Aug 10, 2022

Use the refectored CLI in IDE2.

341f264

Ref: arduino/arduino-cli#1828 Signed-off-by: Akos Kitta <[email protected]>

kittaakos pushed a commit to arduino/arduino-ide that referenced this pull request Aug 10, 2022

Use the refectored CLI in IDE2.

557661c

Ref: arduino/arduino-cli#1828 Signed-off-by: Akos Kitta <[email protected]>

kittaakos pushed a commit to arduino/arduino-ide that referenced this pull request Aug 10, 2022

Use the refectored CLI in IDE2.

2544b9a

Ref: arduino/arduino-cli#1828 Signed-off-by: Akos Kitta <[email protected]>

kittaakos mentioned this pull request Aug 10, 2022

Use Task to build pinned CLI for IDE arduino/arduino-ide#1315

Merged

4 tasks

legacy: Removed ToolsLoader step

7ce9fd4

It has been splitted and merged into HardwareLoader and TargetBoardResolver that are more appropriate.

cmaglie force-pushed the allow_core_search_and_init branch from de6ae8c to 6e00737 Compare August 10, 2022 12:52

cmaglie changed the base branch from master to daemon-fixes August 10, 2022 12:53

cmaglie marked this pull request as ready for review August 10, 2022 13:12

umbynos suggested changes Aug 11, 2022

View reviewed changes

per1234 added topic: code Related to content of the project itself topic: gRPC Related to the gRPC interface and removed topic: CLI Related to the command line interface labels Aug 11, 2022

per1234 requested changes Aug 11, 2022

View reviewed changes

cmaglie added 4 commits August 22, 2022 23:09

Fixed some function comments

8bec66e

Thread-safe protect access to instances map

1d3b99e

cmaglie force-pushed the allow_core_search_and_init branch 3 times, most recently from 7ed5863 to 721e407 Compare August 22, 2022 23:30

cmaglie added 10 commits August 23, 2022 12:31

PlatformInstall/Uninstall must release PackageManager.Explorer before…

6d6210c

… calling commands.Init Otherwise, since Init will try to take a write-lock, it will block indefinitely.

Moved commands.InstanceContainer -> rpc.InstanceCommand

3b39d7c

Created a coreInstancesContainer

4db186e

This container will handle all the atomic access to the instances map.

Made CoreInstance.PackageManager field private

dd0414f

Moved the reminder of PackageManager functions to Explorer or Builder

007a0ff

Now GetPackageManager accepts an rpc.InstanceCommand

316d1d3

It has also been deprecated in favor of GetPackageManagerExplorer.

Now GetLibraryManager accepts an rpc.InstanceCommand

dbd6b01

Refactored automatic builtin-tool installation

6438e58

Added gRPC LibraryUpgrade call and fixed 'lib upgrade' command

e8ef962

cmaglie force-pushed the allow_core_search_and_init branch 2 times, most recently from 249193a to 23694ca Compare August 23, 2022 13:58

cmaglie requested review from umbynos and per1234 August 23, 2022 14:03

Updated documentation

04e70c5

cmaglie force-pushed the allow_core_search_and_init branch from 23694ca to 04e70c5 Compare August 23, 2022 14:09

per1234 requested changes Aug 24, 2022

View reviewed changes

cmaglie force-pushed the allow_core_search_and_init branch from 45844a8 to afbed02 Compare August 24, 2022 07:58

Apply suggestions from code review

df28c7c

Co-authored-by: per1234 <[email protected]>

cmaglie force-pushed the allow_core_search_and_init branch from 009dc94 to df28c7c Compare August 24, 2022 07:59

per1234 approved these changes Aug 26, 2022

View reviewed changes

cmaglie merged commit 276b0cc into arduino:daemon-fixes Aug 26, 2022

cmaglie deleted the allow_core_search_and_init branch August 26, 2022 10:42

cmaglie mentioned this pull request Aug 30, 2022

Investigate possible concurrency issue in gRPC daemon during Init #1801

Closed

3 tasks

kittaakos mentioned this pull request Sep 1, 2022

#1366 Buffer events from BoardListWatchResponse arduino/arduino-ide#1379

Closed

4 tasks

per1234 mentioned this pull request Sep 1, 2022

"Downloading index" process stalls arduino/arduino-ide#1330

Closed

3 tasks

kittaakos mentioned this pull request Sep 3, 2022

Remove unnecessary board discovery stop/start guard arounds lib/platform install/uninstall commands arduino/arduino-ide#1389

Open

3 tasks

kittaakos mentioned this pull request Dec 11, 2023

[grpc]: error: concurrent map iteration and map write when executing lib list #2457

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[breaking] daemon: Fix concurrency and streamline access to PackageManager #1828

[breaking] daemon: Fix concurrency and streamline access to PackageManager #1828

cmaglie commented Aug 9, 2022 •

edited by ubidefeo

Loading

codecov bot commented Aug 9, 2022 •

edited

Loading

cmaglie commented Aug 10, 2022

umbynos Aug 11, 2022

cmaglie Aug 23, 2022

per1234 left a comment

[breaking] daemon: Fix concurrency and streamline access to PackageManager #1828

[breaking] daemon: Fix concurrency and streamline access to PackageManager #1828

Conversation

cmaglie commented Aug 9, 2022 • edited by ubidefeo Loading

codecov bot commented Aug 9, 2022 • edited Loading

Codecov Report

cmaglie commented Aug 10, 2022

umbynos Aug 11, 2022

Choose a reason for hiding this comment

cmaglie Aug 23, 2022

Choose a reason for hiding this comment

per1234 left a comment

Choose a reason for hiding this comment

cmaglie commented Aug 9, 2022 •

edited by ubidefeo

Loading

codecov bot commented Aug 9, 2022 •

edited

Loading